Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: date selection in filters #140

Merged
merged 4 commits into from
Aug 8, 2021

Conversation

nmcharlton
Copy link
Collaborator

Resolves #94

A null date passed into KeyboardDatePicker as minDate or maxDate causes the back and forward buttons respectively to be permanently disabled.

1900-01-01 and 2100-01-01 are the default values for these properties (see https://material-ui-pickers.dev/api/DatePicker).

@@ -285,7 +285,7 @@ function Filter(props) {
value={dateStart}
onChange={handleDateStartChange}
format={getDateFormatLocale(true)}
maxDate={dateEnd}
maxDate={dateEnd || Date('2100-01-01')}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could add these default values to the state on line 58-59 so we can keep the initial state and it's defaults in one place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I've understood your suggestion correctly, that doesn't have quite the desired result.
We want to keep the actual dateStart and dateEnd unset so that no value is selected in the input.
It's only the minDate and maxDate props that we need to set.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I misunderstood! Ok, that solution works. What do you think about adding comments to explain why the dates are there? or to create variables maxDateDefault and minDateDefault? I'm concerned with someone changing or deleting the defaults in a refactor.
This would be a good thing to have a test for...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good shout, thanks. I'll make it a bit more explicit and add a test or two.

@@ -230,7 +230,7 @@ function Filter(props) {
format={getDateFormatLocale(true)}
value={dateStart}
onChange={handleDateStartChange}
maxDate={dateEnd}
maxDate={dateEnd || Date('2100-01-01')}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with Filter.js we can set defaults on lines 72-73 so they are set from the start.

@dadiorchen
Copy link
Collaborator

@nikolamilijevic1 what's your opinion in terms of the usage of the cypress unit test, is it okay? Or there still are some barriers using it.

@nmcharlton nmcharlton marked this pull request as draft August 4, 2021 19:35
@nmcharlton
Copy link
Collaborator Author

The "fix" doesn't work quite as it should; needs a little more work 🤔

@nmcharlton nmcharlton marked this pull request as ready for review August 4, 2021 20:02
@nmcharlton
Copy link
Collaborator Author

Modified behaviour to not allow date selection after today (since all data in the platform is historic), but to allow arbitrary start and end date selection in the past if the other value has not been set.

@nmcharlton nmcharlton merged commit 06a146f into Greenstand:master Aug 8, 2021
@nmcharlton nmcharlton deleted the 94-date-selection branch February 22, 2022 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter date selection: Cannot move forward
3 participants